Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC2184: Allow the use of the HTML <details> tag #2184

Merged
merged 1 commit into from
Mar 14, 2021

Conversation

ananace
Copy link
Contributor

@ananace ananace commented Jul 16, 2019

Rendered

Fixes #2182

@turt2live turt2live added proposal A matrix spec change proposal proposal-in-review labels Jul 16, 2019
@turt2live turt2live changed the title Allow the use of the HTML <details> tag MSC2184: Allow the use of the HTML <details> tag Jul 16, 2019
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for the detailed MSC! It's worth mentioning that the HTML restrictions in the spec are just recommendations (albeit strong ones) rather than requirements. This does change the language of the MSC slightly, but not drastically enough to warrant fixing I don't think.

proposals/2184-allow-html-details.md Outdated Show resolved Hide resolved
@ananace
Copy link
Contributor Author

ananace commented Aug 12, 2019

I was a bit bored, so I did a quick mockup of the functionality by modifying the rendered Riot-web HTML for a message, just to see how it could look without anything more than a change in the allowed tag list.

MCS2182

@turt2live
Copy link
Member

I'm getting myself into the habit of queuing things into the FCP queue:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Aug 12, 2019

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

No concerns currently listed.

Once at least 75% of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Aug 12, 2019
@ara4n
Copy link
Member

ara4n commented Aug 12, 2019

I am torn - on one hand, i really like the idea. on the other hand, i can see it being quite painful for those (e.g. Riot/iOS and Riot/Android and Seaglass) who are wrapping their own HTML renderer currently. And as @ananace quite rightly notes in the MSC, it does collide with #1767, which in theory could provide equivalent behaviour without depending on HTML to do it, and be more flexible in terms of allowing all sorts of different content/fallbacks to be togglable on & off.

What do others think?

@turt2live
Copy link
Member

my perspective is that MSC1767 is a great thing we should definitely do, however in the interest of progress we should shove this one through. The spec does not have a requirement on any of the HTML rendering, it just leaves it as a strong recommendation. This does technically mean that implementations could just add support if they wanted to without the spec, however this seems trivial enough to just merge into the spec's recommendation.

@ananace
Copy link
Contributor Author

ananace commented Aug 12, 2019

I also realized that I forgot to mention that one difference between this MSC and MSC1767 is that <details> would also support recursive usage - as well as the mentioned act of just using it multiple times in a single message.

As a small example of how that could be used for bot responses; (here as an imaginary message in response to some kind of monitoring query - or a notification)

<details>
  <summary>Host: <b>UP</b>, CPU: <b>CRIT</b>, Load: <b>WARN</b></summary>
  <b>CPU</b>: 100%<br/>
  <b>Load</b>: 15.04 7.4 4.2<br/>
  <b>RAM</b>: 24%<br/>
  <details>
    <summary><b>Disk</b>: 20%</summary>
    <ul>
      <li><b>/</b>: 12% 500GB</li>
      <li><b>/home</b>: 24% 1TB</li>
      <li><b>/var/log</b>: 74% 32MB</li>
    </ul>
  </details>
</details>

@anoadragon453
Copy link
Member

I agree with @turt2live in that I don't want to block this on extensible events like so many other things.

We shouldn't block clients from using this just because some have chosen technologies which make ti difficult to implement.

@KitsuneRal
Copy link
Member

KitsuneRal commented Aug 25, 2019

I, too, am torn on this. I fully see the value of that tag but, aside from overlapping (I wouldn't say "colliding") with extensible events, this tag only appeared in HTML5 (I know, I know...) and, as far as I can see, is not supported by IE (Update: Edge can do it, and I wouldn't look at the "old" IE as a reference anymore) . Most shamefully, HTML5 is not represented in the "Supported HTML subset" for rich text in Qt5 either, which means that - on top of poor things @ara4n mentioned - Nheko, Spectral, and Quaternion will have to go through unusual pains to render <details>.

With that said, the fallback is pretty clear: stripping the <details> tag and the <summary> contents entirely allows to still properly render the message. It's probably worth it to add this consideration to MSC.

One alternative to consider is to allow an attribute in m.room.message events that advises clients to render those events initially "collapsed" with a given content shown instead. This would emulate <details> functionality even if HTML is not used at all.

@erikjohnston
Copy link
Member

I, too, am torn on this. I fully see the value of that tag but, aside from overlapping (I wouldn't say "colliding") with extensible events, this tag only appeared in HTML5 (I know, I know...) and, as far as I can see, is not supported by IE. Most shamefully, HTML5 is not represented in the "Supported HTML subset" for rich text in Qt5 either, which means that - on top of poor things @ara4n mentioned - Nheko, Spectral, and Quaternion will have to go through unusual pains to render <details>.

OOI, is there a way of implementing similar behaviour that is easier in Qt5? If not then I'd be tempted to allow this and hop/assume that future Qt version start supporting <details> and that they can fallback fine in the mean time.

@KitsuneRal
Copy link
Member

OOI, is there a way of implementing similar behaviour that is easier in Qt5? If not then I'd be tempted to allow this and hop/assume that future Qt version start supporting <details> and that they can fallback fine in the mean time.

Not really. The best way to emulate this in Qt would probably be parse HTML (ew), find the <details> block and separate it into its own widget/QML-component that expands on clicking. Alternatively, it might be possible to replace <details> with a generic <div> and attach some CSS+JS to it that would expand it.

Regarding "if not" - well, the trouble is that the basic HTML subset used by Qt for simple markup inside its widgets (as opposed to full-blown web-pages that can be displayed using Qt Web Engine, which is a wrapper around Chromium) is considered a thing in itself that "happens" to be a subset of HTML4. How much they would even want to update this basic HTML engine is beyond my knowledge. In a lucky case, Qt 6 will have it, almost surely not in 6.0 though. As it stands, I'm preparing to resort to one of the methods from the previous paragraph; however, since a similar trouble occurs across a few platforms, I wonder how much it's worth it.

@Alch-Emi
Copy link

Alch-Emi commented Feb 6, 2020

I just wanted to chip in that I work in a few communities that make heavy use of content warnings, and a feature like this would be perfect for that usecase. The current alternative would be using spoilers and just add text like (CW: some content warning) before, which isn't a super intuitive way of doing it.

A feature like this that makes it intuitive to add content warnings would be super helpful, and greatly appreciated (especially if clients starting selling it as content warnings, which I think could have a great effect for normalizing cws across the ecosystem, like on masto, but that might be too much to ask for :p )

@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
@KitsuneRal
Copy link
Member

To follow up on my own whinings on the topic - I ended up making the HTML parser (with whichever tools Qt provides) in Quaternion; it's (hopefully) generic enough to (hopefully) be reused across C++/Qt ecosystem (and Qt 6 apparently has all the necessary ingredients), so it unblocks more than just myself. Moreover, the actual hands-on experience on converting between Matrix and Qt subsets tells me that conversions/wrappers of that kind are not terribly bad; so I added my tick to this.

@cloudrac3r
Copy link

Can we PLEASE get this to FCP? I don't believe there are any concerns outstanding.

@mscbot
Copy link
Collaborator

mscbot commented Mar 9, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Mar 9, 2021
@mscbot
Copy link
Collaborator

mscbot commented Mar 14, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Mar 14, 2021
tulir added a commit to maunium/matrix-react-sdk-old that referenced this pull request Mar 14, 2021
@turt2live turt2live merged commit f0170f2 into matrix-org:master Mar 14, 2021
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Mar 14, 2021
@turt2live turt2live self-assigned this Apr 6, 2021
turt2live added a commit that referenced this pull request Apr 6, 2021
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review merged A proposal whose PR has merged into the spec! and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Apr 6, 2021
@turt2live
Copy link
Member

merged 🎉

richvdh pushed a commit that referenced this pull request Aug 23, 2021
MSC2184: Allow the use of the HTML <details> tag
richvdh pushed a commit that referenced this pull request Aug 23, 2021
richvdh pushed a commit that referenced this pull request Aug 27, 2021
MSC2184: Allow the use of the HTML <details> tag
richvdh pushed a commit that referenced this pull request Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Consider allowing the HTML <details> tag in messages
9 participants